Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ContractTxInfo construction into helper functions in Signature Request Methods #260

Merged
merged 2 commits into from
Sep 19, 2024
Merged

ContractTxInfo construction into helper functions in Signature Request Methods #260

merged 2 commits into from
Sep 19, 2024

Conversation

wthrajat
Copy link
Collaborator

@wthrajat wthrajat commented Sep 4, 2024 •
edited
Loading

This PR refactors the construction of ContractTxInfoForSender and ContractTxInfoForRecvr into separate helper functions:

  • build_contract_tx_info_for_sender: Handles the construction of the txs_info vector in the req_sigs_for_sender_once function. This encapsulates the logic for preparing the sender's contract transaction information, reducing clutter in the main function.

  • build_contract_tx_info_for_recvr: Constructs the txs vector in the req_sigs_for_recvr_once function. Simplifies the code in the receiver's signature request method, making it easier to understand and maintain.

TLDR:

Solves 2 TODOs:

  1. Take this construction out of function body.
  2. Take the message construction out of the function body.

@wthrajat
separate sender and receiver tx info
cb163ae 
Signed-off-by: wthrajat <rajatkhanduri290102@gmail.com>
@Shourya742
Copy link

@wthrajat Can you squash the commits?

@mojoX911
Copy link

mojoX911 commented Sep 7, 2024

Nack.
There is no benefit of taking these one liners out in their own function.

This is in general a bad idea for maintainability.

@mojoX911
Copy link

@wthrajat if you can just address this #260 (comment) then this is good for merge.

Instead of taking the message construction in a new function, keep them in the old call site itself and move them out from the send function.

@mojoX911 mojoX911 mentioned this pull request Sep 19, 2024
Closed
11 tasks
@wthrajat
fix conflict
12680d5 
Signed-off-by: wthrajat <rajatkhanduri290102@gmail.com>
@wthrajat
Copy link
Collaborator Author

@mojoX911 I’ve removed the redundant TODO in req_sigs_for_sender_once() and moved the message construction outside of the send_message() call in req_sigs_for_recvr_once().

The TODO comment was a bit confusing as I thought it referred to moving the construction outside the parent functions rather than the send_message() calls.

@wthrajat wthrajat self-assigned this Sep 19, 2024
@mojoX911
Copy link

Yes thats my bad. It was confusing. This is good to go now. Merging after the CI passes.

mojoX911
mojoX911 approved these changes Sep 19, 2024
View reviewed changes
Copy link

@mojoX911 mojoX911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@mojoX911 mojoX911 merged commit dfd55b3 into citadel-tech:master Sep 19, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mojoX911 mojoX911 mojoX911 approved these changes

Assignees

@wthrajat wthrajat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wthrajat @Shourya742 @mojoX911